Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

경북대 FE_이은경_3주차 과제 Step1~2 #53

Open
wants to merge 18 commits into
base: dmsrud1218
Choose a base branch
from

Conversation

dmsrud1218
Copy link

@dmsrud1218 dmsrud1218 commented Jul 11, 2024

안녕하세요 멘토님! 항상 꼼꼼한 리뷰 감사합니다!
처음에 커밋을 했는데 새로고침을 해도 올라가지 않아서 테스트로 여러번 올려서 한번에 올라간 점 죄송합니다... 그 뒤로는 제대로 커밋되었습니다!

step1에서 오류가 나는 페이지가 있었어서 이경우 mocklsit를 보여지도록 구현했었는데 멘토님이 의도하신 것이 맞다고 하셨고 이 부분을 다시 수정하지 않고 올린것같아서 수정하여였으며, 홈페이지에서 랭킹순위가 이상하게 제대로 받아오지 못하는 것 같아서 다시 처음부터 진행하여 재업로드 하였습니다..!


<질문>

  1. step2에서 http status에 따라 error다르게 처리하라고하셨는데 처리가 제대로 되었는지 확인하기 위해서는 일부러 error를 내야하는데 이것도 직접 내서 확인을하나요?

  2. 데이터가 없는 경우에 대한 ui 대응을 하는것이 step2에 있었는데 이경우 색다른 UI가 아니라 지금처럼 그냥 에러메세지가 뜨도록 처리하는 것이 맞는지도 궁금합니다..!

  3. 이부분은 추가 질문인데 1주차2주차에서 더 답변을 달지 못했던 것들도 조금씩 혼자 진행을 하고있다면 늦더라도 답을 하는게 좋은가요 아니면 혼자 그냥 정리하면서 이해를 하고 넘어가도 괜찮은지 질문드리고 싶습니다!

항상 감사합니다!🫡

@dmsrud1218 dmsrud1218 changed the title 경북대 FE_이은경_3주차 과제 Step1 경북대 FE_이은경_3주차 과제 Step1~2 Jul 12, 2024
@dmsrud1218
Copy link
Author

step3까지 진행하다가 코드가 많이 섞이고 꼬여서 다시 처음부터 진행하다보니 원본코드와 다른 부분들이 종종 있는것 같습니다.. 하지만 step2까지는 진행하여서 업로드 합니다! 감사합니다!

@dmsrud1218
Copy link
Author

3-4까지 한번에 올렸지만 브랜치로 나누어서 step3,4는 다시 따로 한 번 더 pull request했습니다! 감사합니다. :)

Copy link

@sjoleee sjoleee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고생하셨습니다~ 리뷰 남깁니다!

@@ -12,7 +12,7 @@ type Props = {
onFilterOptionChange: (option: RankingFilterOption) => void;
};

export const GoodsRankingFilter = ({ filterOption, onFilterOptionChange }: Props) => {
export const GoodsRankingFilter: React.FC<Props> = ({ filterOption, onFilterOptionChange }) => {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

왜 React.FC 타입을 사용하셨나요?
React.FC를 사용하는 것과 사용하지 않을때의 차이는 무엇인가요?

Copy link
Author

@dmsrud1218 dmsrud1218 Jul 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

헉... 새로운 도전을 하다가 다시 원래대로 수정하지 않고 올린 것 같습니다. 수정완료하였습니다.

추가적으로 사용하는 것과 사용하지 않았을 때의 공부도 했습니다.
이 타입을 사용하면 props에 기본적으로 children이 들어가는데 타입스크립트를 쓰는 이유는 정확한 타입을 지정해주면서 자바스크릡트 콛의 안전성을 향상 시키는 부분인데 이 타입을 사용하면 타입이 명확하지 않는다는 단점이 있습니다.

{hasMore ? '접기' : '더보기'}
</Button>
</ButtonWrapper>
{goodsList.length > 6 && (
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

6과 같은 숫자는 별도의 상수로 분리하면 더욱 안전하게 사용할 수 있을 것 같아요

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수정완료하였습니다.

src/api/api.ts Outdated
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

프로젝트 규모가 커지고 api 수가 늘어나면 여기에 필요한 함수를 계속 작성하게 되나요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

프로젝트 규모가 커지만 services 폴더나 api를 모아두는 폴더를 따로 만들어서 관리한다고 배웠습니다! 더 커지게되면 폴더로 모아 두고 잘 볼 수 있도록 관리할예정입니다!

useEffect(() => {
if (isFetchingNextPage) return;

if (observerRef.current) observerRef.current.disconnect();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 라인은 어떤 경우에 필요한가요?
cleanup이 있어도 필요한가요?

}, [isFetchingNextPage, fetchNextPage, hasNextPage]);

if (status === 'loading') return <Message>Loading...</Message>;
if (status === 'error') return <Message>{(error as Error).message}</Message>;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

굳이 타입 단언이 필요할까요?

@@ -31,6 +67,8 @@ export const ThemeGoodsSection = ({}: Props) => {
/>
))}
</Grid>
<div ref={loadMoreRef} />
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 div는 로딩중일 경우에는 DOM에서 제거하는 것이 안전하지 않을까요?

Comment on lines +17 to +58
useEffect(() => {
const loadThemeData = async () => {
setIsLoading(true);
setErrorMessage(null);

try {
const themesResponse = await fetchThemes();
const theme = getCurrentTheme(themeKey, themesResponse.themes);
if (!theme) {
setErrorMessage('유효하지 않은 키입니다.');
setIsLoading(false);
return;
}
setCurrentTheme(theme);
setIsLoading(false);
} catch (error) {
if (axios.isAxiosError(error)) {
if (error.response) {
switch (error.response.status) {
case 404:
setErrorMessage('상품을 찾을 수 없습니다.');
break;
case 500:
setErrorMessage('서버 오류가 발생했습니다.');
break;
default:
setErrorMessage('예기치 않은 오류가 발생했습니다.');
}
} else if (error.request) {
setErrorMessage('요청이 있지만 응답을 받지 못한 경우');
} else {
setErrorMessage('오류 설정문제발생');
}
} else {
setErrorMessage('에러가 발생했습니다.');
}
setIsLoading(false);
}
};

loadThemeData();
}, [themeKey]);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

조건문이 많이 사용되어 가독성이 좋지 않은데 개선해볼 수 있을까요~?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants